Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BLUE-271 fix: applyTimestamp added to processed tx #76

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

achal-singh
Copy link
Contributor

@achal-singh achal-singh commented Sep 4, 2024

Summary

applyTimestamp was not set correctly in case of processed transaction logic, this PR fixes that.

@achal-singh achal-singh self-assigned this Sep 4, 2024
@jairajdev jairajdev changed the base branch from BLUE-257 to dev September 4, 2024 17:55
@jairajdev jairajdev changed the base branch from dev to BLUE-257 September 4, 2024 18:00
@jairajdev jairajdev changed the base branch from BLUE-257 to dev September 4, 2024 18:01
src/Data/Data.ts Outdated
// Send ehlo event right after connect:
socketClient.emit('ARCHIVER_PUBLIC_KEY', config.ARCHIVER_PUBLIC_KEY)
archiverKeyisEmitted = true
Logger.mainLogger.debug(`✅ New Socket Connection to consensus node ${node.ip}:${node.port} is made`)

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.
@jairajdev jairajdev changed the title fix: applyTimestamp added to processed tx BLUE-271 fix: applyTimestamp added to processed tx Sep 16, 2024
@@ -1157,7 +1157,7 @@ export const storeAccountData = async (restoreData: StoreAccountParam = {}): Pro
txId: receipt.data.txId || receipt.txId,
cycle: receipt.cycleNumber,
txTimestamp: receipt.timestamp,
txApplyTimestamp: null,
applyTimestamp: receipt.timestamp,
Copy link

@devendra-shardeum devendra-shardeum Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jairajdev what's the difference b/w applyTimestamp and txTimestamp ? both are receiving the same value ..i.e. receipt.timestamp and also what's the reasoning behind renaming applyTimestamp from txApplyTimestamp? I hope it won't break the other flow! in archive-server

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

txTimestamp is the timestamp set before putting the tx in the tx queue
txApplyTimestamp is the timestamp when the tx is processed/consensused

but in this storeAccountData function which was used to restore the tx receipt from the first node in the network ( this feature is not used anymore ), since there is no way we can calculate the txApplyTimestamp, we have to set the txTimestamp.

Copy link

@devendra-shardeum devendra-shardeum Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but what's the need to have 2 fields receiving the same value.. if applyTimestamp value can't be correctly calculated and inserted, why shouldn't it be removed ? what's the purpose of having this extra field then ? @jairajdev

@mhanson-github mhanson-github merged commit 8d8b671 into dev Sep 18, 2024
6 of 7 checks passed
@mhanson-github mhanson-github deleted the applyts-processedTx branch September 23, 2024 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants